Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PCI CAN cards for QEMU: Kvaser and CTU CAN FD #15161

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

szafonimateusz-mi
Copy link
Contributor

Summary

Impact

We can now simulate CAN networks on Linux host with vcan.

Testing

qemu x86_64 and qemu arm32 with host canutils

It works now only with QEMU, for physical cards there is missing bus timings configuration ignored by QEMU implementation.

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Area: Drivers Drivers issues Area: PCI Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Dec 12, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 12, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but could be improved. While it addresses the key points, it lacks detail in several crucial areas.

Areas for Improvement:

  • Summary: While the "what" is described, the "why" is missing. Why is it necessary to add these QEMU-only drivers? What use case do they enable? How exactly do these drivers work (beyond the linked QEMU documentation)?
  • Impact: The impact section is too brief. While it mentions user impact (simulation with vcan), it doesn't address most of the other points. Specifically:
    • Build impact: Does adding these drivers change the build process at all (e.g., new Kconfig options)?
    • Hardware impact: While these are QEMU only now, is the intention to eventually support physical cards? If so, that should be mentioned.
    • Documentation impact: Be more specific about the added documentation. Where is it located? What does it cover?
    • Other impact categories: Explicitly state "NO" for any categories that aren't applicable (security, compatibility, etc.) rather than omitting them. This adds clarity.
  • Testing: The testing section is weak. "It works" is insufficient. Provide more specific test cases. What exactly was tested? What commands were used? Show example output demonstrating successful CAN communication. The "missing bus timings" comment also belongs in the impact section (and potentially the summary as a known limitation). Provide full details of your build host (OS version, compiler version, etc.) and the QEMU configurations used.

Example of Improved Testing Section:

Testing:

Build Host: Ubuntu 22.04, GCC 11.3.0
Target:
  * QEMU x86_64, virt machine,  NuttX configuration: `configs/qemu-x86_64/nsh`
  * QEMU ARMv7-A (Cortex-A15), virt machine, NuttX Configuration: `configs/qemu-a15/nsh`


Tested using the following steps:

1. Started QEMU with the following command (x86_64 example):
   `qemu-system-x86_64 -enable-kvm -m 1024 -nographic -net none -serial stdio -drive file=nuttx.img,if=virtio`

2. Within the NuttX NSH console, started the Kvaser CAN driver:
   `can_kvaser start`

3. On the host system, used `candump` to monitor the `vcan0` interface:
   `candump vcan0`

4. Within the NuttX NSH console, sent a CAN message:
   `cansend vcan0 123#1122334455667788`

5. Observed the message correctly received on the host system via `candump`:
   `vcan0  123   [8] 11 22 33 44 55 66 77 88`

Similar tests were performed for the CTU CAN FD driver and on the ARMv7-A target.

Known Limitation:  Bus timing configuration is not currently implemented and is ignored by the QEMU implementation. This will need to be addressed for support on physical hardware.

By adding more detail and addressing the missing information, you can significantly strengthen the PR and make it easier for reviewers to understand and approve.

@xiaoxiang781216 xiaoxiang781216 merged commit 1525054 into apache:master Dec 13, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Area: Drivers Drivers issues Area: PCI Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants